Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not create class unload PIC site assumption if not required #18063

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Do not create class unload PIC site assumption if not required #18063

merged 1 commit into from
Sep 5, 2023

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Sep 1, 2023

The issue is TR_RelocationRecordSymbolFromManager::activatePointer creates TR_UnloadedClassPicSite for a J9Method in the pre-prologue of the current method body. The class of the J9Method is the same as the class of the current method body. In this case, the class unload assumption is not needed because once the class is unloaded, no code will ever reach the J9Method in the pre-prologue.

Fixes: #17734

@a7ehuo
Do not create class unload PIC site assumption if not required
e16b1ff 
The issue is `TR_RelocationRecordSymbolFromManager::activatePointer`
creates `TR_UnloadedClassPicSite` for a J9Method in the pre-prologue
of the current method body. The class of the J9Method is the same as
the class of the current method body. In this case, the class unload
assumption is not needed because once the class is unloaded, no code
will ever reach the J9Method in the pre-prologue.

Fixes: eclipse-openj9#17734

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo requested a review from mpirvu September 1, 2023 14:38
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 1, 2023

@mpirvu @klangman May I ask you to review this change? Thank you!
@hzongaro fyi

mpirvu
mpirvu approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT

@mpirvu
Copy link
Contributor

mpirvu commented Sep 1, 2023

Are there other parts of the code that may need a similar change?

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 1, 2023

#17734 is only reproduced so far with AOT loaded methods. Other places in RelocationRecord.cpp that call addPICtoPatchPtrOnClassUnload to create the assumption already check either _needUnloadAssumption from relocation record private data or call isUnloadAssumptionRequired:

openj9/runtime/compiler/runtime/RelocationRecord.cpp

Lines 2696 to 2698 in 2cbea41

if ( reloRuntime->fej9()->isUnloadAssumptionRequired( classOfInlinedMethod, reloRuntime->comp()->getCurrentMethod() ) )
{
reloTarget->addPICtoPatchPtrOnClassUnload(classOfInlinedMethod, &(inlinedCallSite->_methodInfo));

openj9/runtime/compiler/runtime/RelocationRecord.cpp

Lines 5677 to 5679 in 2cbea41

if (reloPrivateData->_needUnloadAssumption)
{
reloTarget->addPICtoPatchPtrOnClassUnload(reloPrivateData->_clazz, reloLocation);

openj9/runtime/compiler/runtime/RelocationRecord.cpp

Lines 5883 to 5884 in 2cbea41

if (reloPrivateData->_needUnloadAssumption)
reloTarget->addPICtoPatchPtrOnClassUnload(reloPrivateData->_clazz, reloLocation);

No class pointer

openj9/runtime/compiler/runtime/RelocationRecord.cpp

Lines 6138 to 6140 in 2cbea41

TR_RelocationRecordClassUnloadAssumption::applyRelocation(TR_RelocationRuntime *reloRuntime, TR_RelocationTarget *reloTarget, uint8_t *reloLocation)
{
reloTarget->addPICtoPatchPtrOnClassUnload((TR_OpaqueClassBlock *) -1, reloLocation);

klangman
klangman approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@klangman klangman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 1, 2023

I started Jenkins test with this change and having the assert as TR_ASSERT_FATAL to double check if there are other cases that hit this assert.

@a7ehuo a7ehuo changed the title Do not create class unload PIC site assumption if not required WIP: Do not create class unload PIC site assumption if not required Sep 1, 2023
@mpirvu
Copy link
Contributor

mpirvu commented Sep 2, 2023

jenkins test sanity all jdk17

@mpirvu
Copy link
Contributor

mpirvu commented Sep 4, 2023

@a7ehuo Unless the result from the grinder mentioned in comment #18063 (comment) is not good, I am ready to perform the merge.

@mpirvu mpirvu self-assigned this Sep 4, 2023
@mpirvu mpirvu added the comp:jit label Sep 4, 2023
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 5, 2023

Jenkins result looks good. I ran sanity.functional,sanity.system,sanity.openjdk,extended.system,extended.functional on x86-64_linux,ppc64le_linux,s390x_linux,x86-64_mac with this change and the assert as TR_ASSERT_FATAL. The assert does not happen.

@a7ehuo a7ehuo changed the title WIP: Do not create class unload PIC site assumption if not required Do not create class unload PIC site assumption if not required Sep 5, 2023
@mpirvu mpirvu merged commit bbe274b into eclipse-openj9:master Sep 5, 2023
@a7ehuo a7ehuo deleted the pr-fix-creationClassUnload-aotLoad branch March 6, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klangman klangman klangman approved these changes

@mpirvu mpirvu mpirvu approved these changes

Assignees

@mpirvu mpirvu

Labels
comp:jit
Projects
None yet
Milestone
No milestone
3 participants
@a7ehuo @mpirvu @klangman